Skip to content

refactor(routers): remove DiracHttpResponseError and simplify OAuth pending response handling#988

Open
mmascher wants to merge 7 commits into
DIRACGrid:mainfrom
mmascher:exceptions_review
Open

refactor(routers): remove DiracHttpResponseError and simplify OAuth pending response handling#988
mmascher wants to merge 7 commits into
DIRACGrid:mainfrom
mmascher:exceptions_review

Conversation

@mmascher

@mmascher mmascher commented Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown

Summary

This PR updates the auth error-handling approach in routers by removing DiracHttpResponseError and handling the OAuth pending-authorization response directly in the token route.

What changed

  • Removed the custom DiracHttpResponseError path from routers.
  • Returned the OAuth authorization_pending payload directly from the token endpoint using a JSON response.
  • Removed now-unneeded exception handler wiring in app factory.
  • Updated coding conventions docs to reflect the current approach:
    • protocol-specific payloads can be returned directly when HTTPException shape is not suitable.

Why

The custom exception class became a one-off indirection with little value:

  • direct response in the endpoint is clearer,
  • behavior is easier to follow in reviews,
  • less plumbing in global exception handling.

@mmascher
feat(routers): move DiracHttpResponseError to routers layer
4a94a43 
* move DiracHttpResponseError from core exceptions to diracx-routers
* update imports in router factory and auth token router
* keep app-level custom exception handling behavior unchanged
* document error-boundary/fallback intent in coding conventions
* normalize remaining e.args[0] usage to str(e) for consistency
Copilot AI review requested due to automatic review settings July 10, 2026 17:10

This comment was marked as outdated.

Sign in to view

@mmascher
Forgot to inline class name in coding-conventions.md
021ec37 
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
aldbr
aldbr requested changes Jul 13, 2026
View reviewed changes

@aldbr aldbr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your PR!
The documentation looks reasonable. I just a few comments.

Note: the changes you made, or my suggestions, are breaking changes, because extensions could, in theory, already rely on these DiracHttpResponseError and DiracError.

In practice, only LHCb is running an extension in production as of now so it should be ok. We would just need to double check on our side + ask other VOs running diracx extensions in certification if they are already relying on the exceptions.

Comment thread .gitignore Outdated
Comment thread diracx-routers/src/diracx/routers/exceptions.py Outdated
Comment thread docs/dev/reference/coding-conventions.md Outdated
Comment thread diracx-core/src/diracx/core/exceptions.py Outdated
@DIRACGridBot
DIRACGridBot marked this pull request as draft July 13, 2026 07:40
@mmascher

mmascher commented Jul 13, 2026

Copy link
Copy Markdown
Author

@aldbr I didn't realize moving DiracHttpResponseError could be a breaking change! I actually wanted to discuss this on Friday afternoon over Zoom, but as you know, my washing machine repair guy got in the way. 😄

Leaving it in exceptions.py and adding a comment saying it should only be used in routers (and only when HTTPException isn't sufficient) was the other option I considered. I'm happy to go back to that if you're OK with it.

Let's make sure my very first contribution is safe and doesn't break extensions or upset people. 😄

mmascher and others added 2 commits July 13, 2026 14:27
@mmascher @aldbr
Update .gitignore
cbbb738 
Remove local ignored file

Co-authored-by: aldbr <aldbr@outlook.com>
@mmascher
refactor(routers): return OAuth pending response directly in token route
279e32e 
* return JSONResponse directly for authorization_pending
@aldbr aldbr linked an issue Jul 15, 2026 that may be closed by this pull request
DiracError, do we keep it? #576
Open
@mmascher

mmascher commented Jul 15, 2026

Copy link
Copy Markdown
Author

@aldbr I think I am done here, I added commits as a reference and then solved the various threads.

@mmascher
mmascher marked this pull request as ready for review July 15, 2026 15:12
@read-the-docs-community

read-the-docs-community Bot commented Jul 15, 2026

Copy link
Copy Markdown

Documentation build overview

📚 diracx | 🛠️ Build #33602113 | 📁 Comparing c3e1303 against latest (19cb080)

  🔍 Preview build  

1 file changed
± dev/reference/coding-conventions/index.html

@mmascher mmascher changed the title feat(routers): move DiracHttpResponseError to routers layer refactor(routers): remove DiracHttpResponseError and simplify OAuth pending response handling Jul 17, 2026
@mmascher

mmascher commented Jul 17, 2026

Copy link
Copy Markdown
Author

FYI, the test is failing because of disk space: https://github.com/DIRACGrid/diracx/actions/runs/29428374798/job/87396639449?pr=988#step:8:223

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments
@aldbr aldbr aldbr requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

DiracError, do we keep it?

3 participants

@mmascher @aldbr